Replace list comprehensions with generator expressions and similar functions #7440
Conversation
Solomon1732
changed the title
Solomon1732
changed the title
Solomon1732
requested review from
a team and
drew2a
and removed request for
a team
src/tribler/core/components/libtorrent/restapi/downloads_endpoint.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Great work! As a team, we truly appreciate your contribution to improving the quality of our codebase. This PR requires some fine-tuning, such as the addition of tests. If I understand correctly, you might be experiencing difficulties with the Tribler development environment due to the M1(M2) chipset.
If you're unable or prefer not to do this further refinement, I can handle it. Otherwise, please feel free to complete the PR on your own.
|
You understand me correctly 🙂. I'll try to add tests on my own. |
|
How can I tell the test unit it's fine to access protected class members? Static code analysis complains about accesses to protected class members (_safe_extended_peer_info and _filter_characters). But I need to access them to test them. Also I noticed the Windows-latest pytest was canceled and didn't finish? It didn't happen before. |
Removed list comprehensions from calls to `str.join`. Instead, generator expressions or calls to `map` were used. In addition, a function was defined twice in `src/tribler/core/components/libtorrent/torrentdef.py`. It was defined as an internal function inside two different function. It was extracted as an internal module one and used in those two places.
Replaced list comprehensions in calls to `pathlib.Path`. Instead, `map` and generator expressions are used.
Replaced indented `for` loops and `if`` with a generator expression, `filter`, and `set`. Instead of repeatedly appending to a list, it creates a set in one go. This set uses the lazy evaluation of `filter` and of an iterator tool from `itertools`.
Added a test to check `_safe_extended_peer_info` in `download_endpoint.py`.
Refractored the methods `get_trackers_as_single_tuple` in `torrentdef.py`. Renamed them `get_trackers`. Updated their documentation and function signatures. In addition, I updated tests and places where these methods are called. Tests for said call sites were also updated. Replaced `not` with explicit length checks in tests in `test_torrent_def.py`.
Added a test to check `get_trackers` works as expected. It checks whether it returns a flat set with trackers. Fixed the test for `_start_download_from_file`. Also fixed a few docstrings of a few functions.
You should add this comment at the beginning of the file:
This situation can occasionally occur and the reasons are not always clear. You can try rerunning the check, which should resolve the issue. |
How can I rerun it? I'm unfamiliar with the bot commands 😅. Also I don't know if I can even run them. |
|
I'm not sure why the mac build timed out. It's the second time in a row it happened. |
Essentially, you have two options:
This sometimes happens, and the reason is currently unknown. You can find more details on this issue at #7134. For your pull requests, it's generally safe to ignore these timeouts. |
|
Yes, I lack the necessary permissions. Is there something else I need to fix in this PR? Or can I remove the "WIP"? |
|
The PR looks good, except for one minor issue: these comments are somewhat confusing:
I would recommend refraining from explaining the pylint disable (as it should be self-explanatory). Like from: def test_safe_extended_peer_info():
# This test is needed to check a protected method. Therefore, this test
# needs to be disabled
# pylint: disable=protected-access
"""
Test that we return the string mapped by `chr` in the case of `UnicodeDecodeError`
"""
extended_peer_info = download_endpoint._safe_extended_peer_info(b"abcd")
assert extended_peer_info == "abcd"to def test_safe_extended_peer_info():
"""
Test that we return the string mapped by `chr` in the case of `UnicodeDecodeError`
"""
# pylint: disable=protected-access
extended_peer_info = download_endpoint._safe_extended_peer_info(b"abcd")
assert extended_peer_info == "abcd"or even to def test_safe_extended_peer_info():
"""
Test that we return the string mapped by `chr` in the case of `UnicodeDecodeError`
"""
extended_peer_info = download_endpoint._safe_extended_peer_info(b"abcd") # pylint: disable=protected-access
assert extended_peer_info == "abcd"And yes, feel free to remove [WIP] from the PR title :) |
Added a test for the new function `_filter_characters`. Patched `test_get_name_as_unicode`. Before, it depended on manual maintenance of the right amount of `"?"` characters. Now, it is calculated programmatically. This ensures if the length changes in the future, the test doesn't break. Minor coding style fixes. Fixed pylint warning. This is needed so pylint doesn't complain for tests of the functionality of specific protected methods.
Solomon1732
changed the title
Replaced generator expressions with similar functions, such as
mapandfilter. In addition, replaced an indented pair offorloops andifstatement with a creation ofset. It uses a generator expression and a call tofilterto create it in one go. This way it avoids repeatedly appending to a list. Fixes #7433 .